LGTM!

Claude Code Review

Critical Issue: Validation Ordering Bug

Location: packages/sandbox/src/sandbox.ts:2133-2151

Token validation happens after the container API call. Invalid tokens are sent to the container before validation runs:

// Line 2133: Container called FIRST
await this.client.ports.exposePort(
  port,
  sessionId,
  options?.name,
  options?.token  // Could be invalid
);

// Line 2151: Validation SECOND (too late)
if (options.token) {
  this.validateCustomToken(options.token);
  token = options.token;
}

If a user passes token: "INVALID@#$", it reaches the container unchecked. While validation eventually throws, the container has already processed the request.

Move validation before the container call:

// Validate token FIRST
let token: string;
if (options.token) {
  this.validateCustomToken(options.token);
  token = options.token;
} else {
  token = this.generatePortToken();
}

const sessionId = await this.ensureDefaultSession();
await this.client.ports.exposePort(
  port,
  sessionId,
  options?.name,
  options.token
);

Missing Test Coverage

Three critical integration scenarios aren't tested:

1. 63-character boundary: Tests reject 64 chars but don't verify 63 chars works (packages/sandbox/tests/sandbox.test.ts:293)
2. Token storage: No test verifies custom tokens are stored correctly, unlike auto-generated tokens (line 340-346)
3. Request routing: All request-handler tests use 16-char tokens; none test the regex change from {16} to + with variable lengths (packages/sandbox/tests/request-handler.test.ts)

Add tests that verify custom tokens flow through correctly, not just that bad tokens are rejected.

Verdict

Fix the validation ordering before merge. The test gaps should be addressed but aren't blocking.

Claude Code Review

The implementation is well-structured with good validation logic and test coverage. However, there's one critical security issue that must be addressed before merging.

Critical Issue: Token Collision

Security Risk (10/10): The code doesn't prevent multiple ports from using the same custom token. This creates a security vulnerability where users could potentially access wrong ports or hijack preview URLs.

// Currently allowed - creates security issue:
await sandbox.exposePort(8080, { hostname: 'example.com', token: 'mytoken' });
await sandbox.exposePort(8081, { hostname: 'example.com', token: 'mytoken' });
// Both URLs use same token - which port gets accessed?

Required fix: Before storing a custom token (sandbox.ts:2157), check if it's already in use by another port:

if (options.token) {
  const existingTokens = tokens || {};
  const existingPort = Object.entries(existingTokens).find(
    ([p, t]) => t === options.token && p !== port.toString()
  );
  if (existingPort) {
    throw new SecurityError(
      `Token '${options.token}' is already in use by port ${existingPort[0]}.`
    );
  }
  token = options.token;
}

Required test: Add to sandbox.test.ts:

it('should reject exposing different ports with same custom token', async () => {
  await sandbox.exposePort(8080, { hostname: 'example.com', token: 'shared' });
  await expect(
    sandbox.exposePort(8081, { hostname: 'example.com', token: 'shared' })
  ).rejects.toThrow(/already in use/);
});

Other Issues

Token persistence testing: Missing E2E test for token survival across DO hibernation. This is critical for production stability - if tokens don't persist, preview URLs break after sleep/wake cycles.

Regex edge cases: The URL pattern regex needs tests for consecutive separators (abc--def, test__123) to ensure they parse correctly.

What Looks Good

• Comprehensive input validation with clear error messages
• Proper SecurityError usage
• Type consistency across SDK and shared packages
• Good unit test coverage for validation rules
• Well-documented with JSDoc and examples
• Changeset follows project standards

Verdict: Fix the token collision issue, then this is ready to merge.

OpenCode Review

This is a well-implemented feature that adds custom token support for stable preview URLs. The implementation is thoughtful with proper validation, collision detection, and security considerations.

Strengths:

• Proper input validation with helpful error messages
• Token collision prevention across ports
• Good test coverage for edge cases
• Maintains backward compatibility
• URL parsing updated to handle variable token lengths
• Excellent changeset with clear examples

Minor Issues to Address:

1. Type inconsistency: ExposePortRequest type moved from port-client.ts to shared/request-types.ts but doesn't include the new token field. The container layer won't receive custom tokens.

2. Potential race condition: Between lines 2172-2181, there's a gap where another request could claim the same custom token. Consider checking collision after storage read and before container call.

3. Documentation: Request handler comment still mentions "exactly 16 chars" but should reflect "1-16 chars" for custom tokens.

Overall this looks good to merge once the type definition is updated to include the token field in the container API.

Verdict: A few small issues to address first.

OpenCode Review

After reviewing the implementation, this is a well-architected feature that adds custom token support for stable preview URLs.

Strengths:

• Proper input validation with helpful error messages
• Token collision prevention across ports
• Good test coverage for edge cases
• Maintains backward compatibility
• URL parsing updated to handle variable token lengths
• Excellent changeset with clear examples
• Clean separation of concerns: DO handles tokens/URLs, container handles port exposure
• Documentation updated correctly in request handler

Architecture is correct: The token field doesn't need to be in ExposePortRequest because the container layer only manages port exposure, while the Durable Object layer handles token-based URL access control. This separation is appropriate.

Looks good to merge.

OpenCode Review

Looks good overall! The custom token feature for stable preview URLs is well-implemented with proper validation and collision detection.

A few issues to address:

1. Missing ExposePortRequest field: The interface was moved to @repo/shared but doesn't include the token field, creating a type mismatch between SDK and container
2. Documentation clarity: The changeset mentions tokens work "across deployments" but they're stored in DO storage which is ephemeral

The architecture follows established patterns and the security considerations (collision detection, input validation) are solid.

OpenCode Review

Looks good! This is a clean implementation that adds the ability to use custom tokens in exposePort() for stable preview URLs across deployments.

The implementation handles all the important edge cases - token validation, collision detection, and maintains backward compatibility. The timing-safe comparison and input gate protection for storage are correctly implemented. The changeset properly describes the user-facing benefit.

One thing to note: this will require documentation updates since the Cloudflare docs currently show the exposePort() method without the new token parameter. The API reference at /sandbox/api/ports/ and the usage examples in /sandbox/guides/expose-services/ should be updated to document the new optional parameter.

OpenCode Review

This PR adds custom token support to exposePort() for stable preview URLs across deployments. The implementation looks solid with proper validation and security considerations.

Good architectural choices:

• Token collision prevention across ports while allowing re-exposure of same port
• Timing-safe token comparison using crypto.subtle.timingSafeEqual()
• Clear validation with descriptive error messages
• Comprehensive test coverage including edge cases

Security implementation is correct:

• Validates token format (1-16 chars, lowercase a-z, 0-9, -, _)
• Prevents token reuse across different ports
• Uses constant-time comparison to prevent timing attacks

The regex change in request-handler.ts makes sense - relaxing from exactly 16 chars to 1+ chars while maintaining DNS compatibility.

Documentation impact: This change should be documented in the Cloudflare docs since it affects the public API and enables new use cases for production deployments.

Looks good to merge.